aws_ecs_service support deployment_minimum_healthy_percent for DAEMON strategy #6150

mschurenko · 2018-10-14T05:28:09Z

This PR aims to fix #5236

To summarize, I would like to be able to set deployment_minimum_healthy_percent to 50 when using a scheduling strategy of DAEMON. Currently this is not possible as this value being set to nil. The result of this is that deployment_minimum_healthy_percent is set to 0 (AWS default) which results in downtime for a deployment.

This is my first PR to this project, so I might be missing something obvious, but there doesn't appear to be a way to conditionally set a default value in github.com/hashicorp/terraform/helper/schema https://github.com/terraform-providers/terraform-provider-aws/blob/master/vendor/github.com/hashicorp/terraform/helper/schema/schema.go#L37.

Ideally the default value for deployment_minimum_healthy_percent would be 200 for REPLICA strategy and 0 for DAEMON, but I was unable to do that.

As a result if one uses the DAEMON strategy but does not set a value for deployment_minimum_healthy_percent the default of 100 is used and you get this error:

 aws_ecs_service.my_service: InvalidParameterException: Both maximumPercent and minimumHealthyPercent cannot be 100 as this will block deployments.
	status code: 400, request id: 848ec013-cf6e-11e8-8638-07c29e49208f "my_service"

IMO this is better then the current behavior where one cannot set deployment_minimum_healthy_percent at all.

Having said this, I'm hoping there's a better approach to doing this.

Changes proposed in this pull request:

aws_ecs_service resource will use deployment_minimum_healthy_percent when scheduling_strategy is set to DAEMON

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsService_withDaemonSchedulingStrategy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_withDaemonSchedulingStrategy -timeout 120m
=== RUN   TestAccAWSEcsService_withDaemonSchedulingStrategy
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategy (27.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	(cached)

mschurenko · 2018-10-18T16:34:19Z

Hi @bflad. Not sure if anyone has had a chance to look at this yet.

Thanks

mschurenko · 2018-10-25T02:58:10Z

I'm not trying to sound like a jerk or anything, but is this normal to have no response for a PR after 11 days? If this won't be accepted for whatever reason that's fine. I would just like to know. If it's not I'll most likely need to use Cloudformation for this.

bflad · 2018-10-25T03:20:35Z

@mschurenko its likely to be reviewed and accepted (potentially with some changes) given my understanding of the situation. A bunch of HashiCorp and community members have been busy with preparation, travel, and attendance of HashiConf this week.

mschurenko · 2018-10-25T04:18:23Z

Hey @bflad thanks a lot for the response. Sorry to be so pushy. I didn't realize HashiConf was next week.

Thanks!

bflad

Initial comments below. Please let us know if you have any questions or do not have time to implement the feedback.

bflad · 2018-10-30T17:14:09Z

aws/resource_aws_ecs_service.go

@@ -106,12 +106,6 @@ func resourceAwsEcsService() *schema.Resource {
 				Type:     schema.TypeInt,
 				Optional: true,
 				Default:  100,


Instead of leaving a Default: 100 here that only works for one of the scheduling strategies, should we instead remove Default: 100 and implement a DiffSuppressFunc that covers suppressing the difference depending on the strategy? A similar update should likely occur for maximum as well.

If we remove the Default wouldn't that be backwards incompatible for those who are using a replica strategy and didn't set deployment_minimum_healthy_percent?

Similar for deployment_maximum_percent, if we remove Default won't that be a problem for backwards compatibility?

The port attribute of the aws_elasticache_cluster resource is an example of a place where we allow two defaults chosen by the API, but suppress the difference if it is left unconfigured in the Terraform configuration: https://github.com/terraform-providers/terraform-provider-aws/blob/4ef574a20eeecd860ef40bf4ca94849cee9af0ef/aws/resource_aws_elasticache_cluster.go#L146-L157

Its not pretty, but it works. 😄 We could also set Computed: true.

Ok. So when you say we allow two defaults chosen by the API, do you mean by the AWS API?

bflad · 2018-10-30T17:15:40Z

aws/resource_aws_ecs_service.go

@@ -354,7 +348,7 @@ func resourceAwsEcsServiceCreate(d *schema.ResourceData, meta interface{}) error
 	input.SchedulingStrategy = aws.String(schedulingStrategy)
 	if schedulingStrategy == ecs.SchedulingStrategyDaemon {
 		// unset these if DAEMON
-		input.DeploymentConfiguration = nil
+		input.DeploymentConfiguration.MaximumPercent = aws.Int64(100)


Is there a reason to keep this logic here at all? Since the comment eludes to "unsetting" values due to DAEMON we should either remove this line or add a comment why its being hardcoded to 100

aws/resource_aws_ecs_service_test.go

…maximum_percent

…Minimum acceptance test

mschurenko · 2018-10-31T04:23:09Z

@bflad I made your recommended changes. I removed default values for deployment_maximum_percent and deployment_minimum_healthy_percent so that the AWS API default values are now used.

I added another acceptance test so there is now TestAccAWSEcsService_withDaemonSchedulingStrategy and TestAccAWSEcsService_withDaemonSchedulingStrategySetDeploymentMinimum.

I ran all the ecs acceptance tests and they have passed:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsService_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_ -timeout 120m
=== RUN   TestAccAWSEcsService_withARN
--- PASS: TestAccAWSEcsService_withARN (29.85s)
=== RUN   TestAccAWSEcsService_basicImport
--- PASS: TestAccAWSEcsService_basicImport (36.27s)
=== RUN   TestAccAWSEcsService_disappears
--- PASS: TestAccAWSEcsService_disappears (16.57s)
=== RUN   TestAccAWSEcsService_withUnnormalizedPlacementStrategy
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (43.69s)
=== RUN   TestAccAWSEcsService_withFamilyAndRevision
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (39.37s)
=== RUN   TestAccAWSEcsService_withRenamedCluster
--- PASS: TestAccAWSEcsService_withRenamedCluster (77.87s)
=== RUN   TestAccAWSEcsService_healthCheckGracePeriodSeconds
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (244.06s)
=== RUN   TestAccAWSEcsService_withIamRole
--- PASS: TestAccAWSEcsService_withIamRole (127.35s)
=== RUN   TestAccAWSEcsService_withDeploymentValues
--- PASS: TestAccAWSEcsService_withDeploymentValues (35.34s)
=== RUN   TestAccAWSEcsService_withLbChanges
--- PASS: TestAccAWSEcsService_withLbChanges (231.07s)
=== RUN   TestAccAWSEcsService_withEcsClusterName
--- PASS: TestAccAWSEcsService_withEcsClusterName (37.22s)
=== RUN   TestAccAWSEcsService_withAlb
--- PASS: TestAccAWSEcsService_withAlb (220.44s)
=== RUN   TestAccAWSEcsService_withPlacementStrategy
--- PASS: TestAccAWSEcsService_withPlacementStrategy (104.52s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints
--- PASS: TestAccAWSEcsService_withPlacementConstraints (36.67s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints_emptyExpression
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (14.07s)
=== RUN   TestAccAWSEcsService_withLaunchTypeFargate
--- PASS: TestAccAWSEcsService_withLaunchTypeFargate (312.88s)
=== RUN   TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (81.87s)
=== RUN   TestAccAWSEcsService_withDaemonSchedulingStrategy
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategy (25.51s)
=== RUN   TestAccAWSEcsService_withDaemonSchedulingStrategySetDeploymentMinimum
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategySetDeploymentMinimum (18.01s)
=== RUN   TestAccAWSEcsService_withReplicaSchedulingStrategy
--- PASS: TestAccAWSEcsService_withReplicaSchedulingStrategy (36.20s)
=== RUN   TestAccAWSEcsService_withServiceRegistries
--- PASS: TestAccAWSEcsService_withServiceRegistries (48.68s)
=== RUN   TestAccAWSEcsService_withServiceRegistries_container
--- PASS: TestAccAWSEcsService_withServiceRegistries_container (38.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1856.477s

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsServiceDataSource_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsServiceDataSource_ -timeout 120m

=== RUN   TestAccAWSEcsServiceDataSource_basic
--- PASS: TestAccAWSEcsServiceDataSource_basic (13.67s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	15.621s

Also, I updated the aws_ecs_service html doc to reflect the changes.

bflad

LGTM, thanks @mschurenko! 🚀

--- PASS: TestAccAWSEcsService_withARN (9.45s)
--- PASS: TestAccAWSEcsService_basicImport (11.82s)
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (18.89s)
--- PASS: TestAccAWSEcsService_disappears (19.28s)
--- PASS: TestAccAWSEcsService_withRenamedCluster (20.57s)
--- PASS: TestAccAWSEcsService_withDeploymentValues (10.93s)
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (41.15s)
--- PASS: TestAccAWSEcsService_withEcsClusterName (19.00s)
--- PASS: TestAccAWSEcsService_withLbChanges (38.46s)
--- PASS: TestAccAWSEcsService_withIamRole (119.67s)
--- PASS: TestAccAWSEcsService_withPlacementConstraints (6.95s)
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (10.89s)
--- PASS: TestAccAWSEcsService_withPlacementStrategy (75.45s)
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (47.44s)
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategy (6.69s)
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategySetDeploymentMinimum (6.61s)
--- PASS: TestAccAWSEcsService_withReplicaSchedulingStrategy (10.82s)
--- PASS: TestAccAWSEcsService_withServiceRegistries (16.71s)
--- PASS: TestAccAWSEcsService_withServiceRegistries_container (12.65s)
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (247.67s)
--- PASS: TestAccAWSEcsService_withAlb (231.25s)
--- PASS: TestAccAWSEcsService_withLaunchTypeFargate (262.40s)

bflad · 2018-11-01T00:21:25Z

This has been released in version 1.42.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-02T17:43:54Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

mschurenko added 2 commits October 13, 2018 22:09

enabling MinimumHealthyPercent for DAEMON

213d964

setting deployment_minimum_healthy_percent in test

3985d60

ghost added size/XS Managed by automation to categorize the size of a PR. service/ecs Issues and PRs that pertain to the ecs service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 14, 2018

mschurenko changed the title ~~[WIP] aws_ecs_service support deployment_minimum_healthy_percent for DAEMON strategy~~ aws_ecs_service support deployment_minimum_healthy_percent for DAEMON strategy Oct 15, 2018

bflad added enhancement Requests to existing resources that expand the functionality or scope. waiting-response Maintainers are waiting on response from community or contributor. labels Oct 30, 2018

bflad reviewed Oct 30, 2018

View reviewed changes

mschurenko added 4 commits October 30, 2018 16:01

using nil value for input.DeploymentConfiguration.MaximumPercent

f7983bf

using AWS API defaults for deployment_maximum_percent and deployment_…

7306998

…maximum_percent

adding TestAccAWSEcsService_withDaemonSchedulingStrategySetDeployment…

dd69f6a

…Minimum acceptance test

updating ecs service html markdown

7fda67f

ghost added size/M Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. and removed size/XS Managed by automation to categorize the size of a PR. labels Oct 31, 2018

bflad approved these changes Oct 31, 2018

View reviewed changes

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 31, 2018

bflad added this to the v1.42.0 milestone Oct 31, 2018

bflad merged commit 97bb163 into hashicorp:master Oct 31, 2018

bflad added a commit that referenced this pull request Oct 31, 2018

Update CHANGELOG for #6150 and fix some 1.42.0 ordering

9f6918e

This was referenced Nov 1, 2018

[regression in 1.42] aws_ecs_service min/max deployment percent error #6315

Closed

resource/aws_ecs_service: Continue supporting replica deployment_minimum_healthy_percent = 0 and deployment_maximum_percent = 100 #6316

Merged

ghost locked and limited conversation to collaborators Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws_ecs_service support deployment_minimum_healthy_percent for DAEMON strategy #6150

aws_ecs_service support deployment_minimum_healthy_percent for DAEMON strategy #6150

mschurenko commented Oct 14, 2018 •

edited

Loading

mschurenko commented Oct 18, 2018

mschurenko commented Oct 25, 2018

bflad commented Oct 25, 2018

mschurenko commented Oct 25, 2018

bflad left a comment

bflad Oct 30, 2018

mschurenko Oct 30, 2018

bflad Oct 31, 2018

mschurenko Oct 31, 2018 •

edited

Loading

bflad Oct 30, 2018

mschurenko commented Oct 31, 2018

bflad left a comment

bflad commented Nov 1, 2018

ghost commented Apr 2, 2020

aws_ecs_service support deployment_minimum_healthy_percent for DAEMON strategy #6150

aws_ecs_service support deployment_minimum_healthy_percent for DAEMON strategy #6150

Conversation

mschurenko commented Oct 14, 2018 • edited Loading

mschurenko commented Oct 18, 2018

mschurenko commented Oct 25, 2018

bflad commented Oct 25, 2018

mschurenko commented Oct 25, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Oct 30, 2018

Choose a reason for hiding this comment

mschurenko Oct 30, 2018

Choose a reason for hiding this comment

bflad Oct 31, 2018

Choose a reason for hiding this comment

mschurenko Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

bflad Oct 30, 2018

Choose a reason for hiding this comment

mschurenko commented Oct 31, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Nov 1, 2018

ghost commented Apr 2, 2020

mschurenko commented Oct 14, 2018 •

edited

Loading

mschurenko Oct 31, 2018 •

edited

Loading